-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ODP REST API for Sending ODP Events #315
Conversation
@msohailhussain Let me know if you want to pair up over this review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not reviewed unit test. partial review to unblock if need to work on the feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A few changes suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A clarification regarding TLS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @jaeopt . Holding for @msohailhussain 's approval 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please address few more questions.
@mikechu-optimizely Overall your PR looks good, please address 4 more questions and then lgtm. |
last but not least please fix Lint issues. |
It seems like the lint error here in the GitHub Action is about a missing auth token. Would you like me to pause the Pull Request and resolve this step, so we can make the "Csharp CI with .NET / Lint Code Base (pull_request)" required? ...or are there lint issues that I missed in the code? Can you direct me to an example file? |
this is fine for me, we should resolve token issue in future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Summary
Adding module to provide an internal service for sending events to ODP's REST API.
Test plan
Issues